Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TouchID Implementation [iOS Sim Only] #509

Merged
merged 21 commits into from
Nov 18, 2016
Merged

TouchID Implementation [iOS Sim Only] #509

merged 21 commits into from
Nov 18, 2016

Conversation

dpgraham
Copy link
Contributor

@dpgraham dpgraham commented Nov 7, 2016

Change list

Added touchId command

Types of changes

What types of changes are you proposing/introducing to Java client?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

Touch ID is a feature in Apple products (see https://support.apple.com/en-ca/HT201371). This endpoint simulates the behaviour.

Tests were failing because of an indentation problem in
IOSGesturesTest.java and because a static import in
XCUIAutomationTest.java was coming after the rest of the imports instead
of before
(See https://support.apple.com/en-ca/HT201371 for description of the Touch ID feature)
This endpoint simulates the TouchID feature.
@dpgraham
Copy link
Contributor Author

dpgraham commented Nov 7, 2016

*Not sure why the README.md was diffed that way. I only added one line under the 'Added Functions' header

@@ -31,8 +34,6 @@

import java.io.File;

import static org.junit.Assert.assertEquals;

public class XCUIAutomationTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dpgraham Can you add some tests here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the test. It doesn't do any assertions because there isn't anything to check, it just calls the methods to verify that there aren't any exceptions.

/**
* This method forms a {@link java.util.Map} of parameters for the touchId simulator
*
* @param match Are we simulating a successful fingerprint scan?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it a question here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's how I've documented boolean parameters in the past. I'll change it to be more in line with how it's normally documented.

*
* @param match Are we simulating a successful fingerprint scan?
*/
default void touchId(boolean match) {
Copy link
Member

@SrinivasanTarget SrinivasanTarget Nov 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add in doc as it is Simulator only?

- endTestCoverage()
- isLocked()
- shake()
- touchId()
Copy link
Member

@SrinivasanTarget SrinivasanTarget Nov 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert this change? We will apply this change post release.

@SrinivasanTarget SrinivasanTarget changed the title Touch TouchID Implementation [iOS Sim Only] Nov 8, 2016
@SrinivasanTarget SrinivasanTarget added this to the 5.0.0 milestone Nov 8, 2016
drapostolos and others added 6 commits November 8, 2016 15:37
This will be added post release
This test only executes the driver.touchId method, it doesn't do any assertions. Just verifies that it doesn't throw any exceptions.
@dpgraham
Copy link
Contributor Author

dpgraham commented Nov 8, 2016

Thanks @SrinivasanTarget, I made the changes you requested. If you could review them and they're good let me know so that I can squash the changes and merge them.

driver.touchId(false);
assertEquals(true, true);
} catch(Exception e){
assertEquals(true, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dpgraham I think it is more senseful to throw an exception.
Otherwice we have a risk to produce and invalid build or to not react to server changes.

@@ -56,5 +57,14 @@ default void hideKeyboard(String strategy, String keyName) {
default void shake() {
CommandExecutionHelper.execute(this, shakeCommand());
}

Copy link
Contributor

@TikhomirovSergey TikhomirovSergey Nov 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dpgraham

I am against the adding to the IOSDeviceActionShortcuts. This interface is going to be deprecated. Please take a look at this working branch. IOSDeviceActionShortcuts is going to be deprecated.

It is better to add such interface:

package io.appium.java_client.ios;

public interface ChecksTouchId extends ExecutesMethod {

    /**
    * Simulate touchId event
    *
    * @param match If true, simulates a successful fingerprint scan. If false, simulates a failed fingerprint scan
    */     
    default void touchId(boolean match) {
        CommandExecutionHelper.execute(this, touchIdCommand(match));
    }
}

and make IOSDriver implement it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the input Sergey. I made the change.

@TikhomirovSergey
Copy link
Contributor

@dpgraham
Sorry for the delaying. Usually busy. Please take a look at remarks.

try {
driver.touchId(true);
driver.touchId(false);
assertEquals(true, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actual Usecase of API touchID is to unlock or sign in to any application using touchID. Since we dont have our test app built with any registration or sign in feature, we cannot use that scenario.

Otherwise Can we lock the simulator and unlock the simulator using driver.touchID(true).Ideal case this should be working fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So test scenario should be something like,

   try {
            driver.lockDevice(2);
            driver.touchId(true);
        } catch (Exception e) {
            assertTrue(false);
        }

/**
* Simulate touchId event
*
* @param match If true, simulates a successful fingerprint scan. If false, simulates a failed fingerprint scan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i'm not wrong, this will fail as per google style each doc line should end with a period.

@SrinivasanTarget
Copy link
Member

@dpgraham I've reviewed again now. Please take a look.

Tests were failing because of an indentation problem in
IOSGesturesTest.java and because a static import in
XCUIAutomationTest.java was coming after the rest of the imports instead
of before
(See https://support.apple.com/en-ca/HT201371 for description of the Touch ID feature)
This endpoint simulates the TouchID feature.
This will be added post release
This test only executes the driver.touchId method, it doesn't do any assertions. Just verifies that it doesn't throw any exceptions.
-Moved touchID out of IOSDeviceActionShorcuts and into PerformsTouchID
-Made IOSDriver implement PerformsTouchID
-Fixed typo that I noticed in CommandExecutionHelper
-Fixed linting errors
@dpgraham
Copy link
Contributor Author

I've updated the code again

driver.performTouchID(false);
assertEquals(true, true);
} catch (Exception e) {
assertEquals(true, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test should throw the exception.

At least you can do that

try {
//do something
//assert the result
} catch (Exception e) {
    throw e;
} finally {
//attemt to stabilize the simulator
//or something else
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dpgraham Can you update tests here as suggested please?

/**
* This method forms a {@link java.util.Map} of parameters for the touchId simulator
*
* @param match If true, simulates a successful fingerprint scan. If false, simulates a failed fingerprint scan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc should end with a period everywhere.

@dpgraham
Copy link
Contributor Author

Changes made

@TikhomirovSergey
Copy link
Contributor

TikhomirovSergey commented Nov 17, 2016

@dpgraham Everuthing is ok for me. But could you please improve the proposed test the way I have asked?
@SrinivasanTarget ?

@TikhomirovSergey TikhomirovSergey merged commit 3e9c949 into master Nov 18, 2016
@dpgraham dpgraham deleted the touch-id branch November 18, 2016 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants